Reorganize cuDF Python docs - #10691
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Codecov Report
@@ Coverage Diff @@
## branch-22.06 #10691 +/- ##
================================================
+ Coverage 86.40% 86.43% +0.02%
================================================
Files 143 143
Lines 22444 22448 +4
================================================
+ Hits 19393 19402 +9
+ Misses 3051 3046 -5
Continue to review full report at Codecov.
|
| @@ -2,15 +2,16 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
This looks to be an issue with the way ReviewNB diffs things. There's no ==================================== in the source. Ditto in many places below.
Reply via ReviewNB
| @@ -2,46 +2,31 @@ | |||
| "cells": [ | |||
There was a problem hiding this comment.
I removed the hardcoded table of contents below as Sphinx/myst-nb already generate one for us.
Reply via ReviewNB
galipremsagar
left a comment
There was a problem hiding this comment.
Overall looks great to me Ashwin. Except for a few rendering issues that I mentioned below.
| :class: io-supported-types-table special-table | ||
| :widths: 15 10 10 10 10 10 10 10 10 10 10 10 10 10 10 10 | ||
|
|
||
| +-----------------------+--------+--------+--------+--------+---------+--------+--------+--------+--------+-------------------+--------+--------+---------+---------+ |
There was a problem hiding this comment.
I like how GitHub thinks you only moved this and 2 more files while you also made similar file movements in this PR 🤣
There was a problem hiding this comment.
Yeah what the heck is that about...
galipremsagar
left a comment
There was a problem hiding this comment.
Changes LGTM @shwina and all the issues are now fixed. Thank you!
bdice
left a comment
There was a problem hiding this comment.
Really nice job @shwina. I haven't had a chance to render the docs locally, but I thought I would take a peek. I left a handful of comments, but only one (on index.md) is probably blocking. Feel free to skip/ignore any comments or deal with them in future PRs. I recognize this PR is huge so I don't want to block it. Hope the feedback is helpful! :)
| also provide special data types for working with decimals, list-like, | ||
| and dictionary-like data. | ||
|
|
||
| All data types in cuDF are [nullable](/user_guide/missing-data). |
There was a problem hiding this comment.
This should use the MyST equivalent of Sphinx's :ref: rather than an absolute path. The absolute path won't work in rendered HTML unless it's being served at the root. That's prone to breakage. This piece of the documentation might help: https://myst-parser.readthedocs.io/en/latest/syntax/syntax.html#targets-and-cross-referencing
There was a problem hiding this comment.
Used a relative path instead
There was a problem hiding this comment.
A relative path is an improvement but not quite as good as ref syntax. If the page is moved or renamed, the ref will still pick it up as long as it's in the same Sphinx project. refs with missing referents will throw warnings, and refs are supported by non-HTML Sphinx outputs. An absolute/relative path won't be checked for correctness afaik.
There was a problem hiding this comment.
With markdown syntax [nullable](mossing-data), I get:
cudf/docs/cudf/source/user_guide/data-types.md:8: WARNING: 'myst' reference target not found: mossing-data
The markdown syntax translates into Sphinx's any role, which seems pretty flexible in what it can do.
There was a problem hiding this comment.
Nice to know! If it's translated to the any role, that's awesome. I thought it would be interpreted as a link, rather than a reference (any will match ref or any other role).
(also "mossing data" 😆 )
| ## NumPy data types | ||
|
|
||
| We use NumPy data types for integer, floating, datetime, timedelta, | ||
| and string data types. Thus, just like in NumPy, |
There was a problem hiding this comment.
Is it weird that this always uses two spaces between sentences? Personally I prefer one space between sentences.
There was a problem hiding this comment.
This is just Emacs'Emacs' auto-fill at work. Personally, I think Semantic Linefeeds are the best convention for documentation source, but it's hard to incorporate them in anything but personal projects :(
There was a problem hiding this comment.
Vim’s “gq” does the same thing with two spaces. I would be open to rewriting this with semantic lines (or the easier to enforce “one sentence per line”). I have had no issues in practice with using that style on other projects. This word wrap is pretty harsh right now - it’s wrapping at less than 80 chars (74?), so I think it is unlikely that this word wrapped formatting will survive more than one or two revisions anyway.
There was a problem hiding this comment.
Perhaps a discussion to have offline/as part of the dev guide?
There was a problem hiding this comment.
I would not oppose making that change in this PR, since all the files are being rewritten anyway. If you don't want to make that decision in this PR, this would be a good topic for tomorrow's cuDF Python meeting.
| `np.dtype("float32")`, `np.float32`, and `"float32"` are all acceptable | ||
| ways to specify the `float32` data type: | ||
|
|
||
| ```python |
There was a problem hiding this comment.
I discovered recently that code blocks using >>> are (depending on the renderer) supposed to be rendered as pycon: Python console, rather than python.
That applies to GitHub Flavored Markdown via Linguist and also content highlighted with Pygments. It appears that MyST uses Pygments.
There was a problem hiding this comment.
TIL! I can confirm that it works, but are there any advantages to using pycon over python (either ways, I'd prefer to change this in a follow up)
There was a problem hiding this comment.
Ah I see, things like keywords in output/tracebacks aren't syntax highlighted by mistake. That's really cool!
There was a problem hiding this comment.
Yep! It was definitely a "TIL" thing for me too. I think a lot of projects just use python and don't know the difference.
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
|
Rerun tests |
|
@gpucibot merge |
This PR is composed of two high-level changes:
Replaces the use of ReStructuredText with MyST Markdown. I used rst2myst for this and it worked pretty well. The rationale for this change is simple: we use
myst-nbto render notebooks into documentation, and for consistency, it's nice to usemyst-parserto parse the rest of our docs too. As a matter of opinion, I think Markdown is simpler and more familiar to most developers.Reorganizes the docs (see below):
Prior to this PR, the cuDF documentation was divided into 3 sections:
The distinction between the first two sections was never clear. I've gone ahead and merged those into a single section named "User Guide". This is also more consistent with Pandas.
This PR also makes a couple of other changes:
Compare the TOC from this PR (below) with our currently published docs.